Add ECDSA Support (#530) - #531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe library adds ECDSA-SHA256 and ECDSA-SHA512 algorithms, registers their XML identifiers, updates public types, and adds signing examples, output fixtures, documentation, and verification tests. ChangesECDSA algorithm support
Examples and documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change adds ECDSA SHA-256 and SHA-512 XML signing and verification support with coverage for valid signatures and invalid or mismatched inputs. No current merge-readiness risk is identified. Sequence Diagram(s)sequenceDiagram
participant Caller
participant SignedXml
participant ECDSAKey
participant XMLDocument
Caller->>SignedXml: configure ECDSA algorithm and reference
SignedXml->>XMLDocument: canonicalize referenced content
SignedXml->>ECDSAKey: sign canonicalized content
ECDSAKey-->>SignedXml: return IEEE P1363 signature
SignedXml-->>Caller: return signed XML
Caller->>SignedXml: load signature and public certificate
SignedXml->>ECDSAKey: verify SignatureValue
ECDSAKey-->>SignedXml: return verification result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@example/local_example.js`:
- Around line 4-7: Replace the relative require("../") that imports SignedXml
with the package name require("xml-crypto") so the example demonstrates end-user
usage; locate the SignedXml import in local_example.js (currently assigned to
the SignedXml variable) and update that require to "xml-crypto" while leaving
other requires (select, DOMParser, fs) unchanged.
🧹 Nitpick comments (5)
src/signature-algorithms.ts (2)
56-84: Consider removing@ts-ignoreby usingcrypto.createPrivateKey.The commented-out fix on lines 59-60 would work. Using
crypto.createPrivateKey(privateKey)ensures the key is properly parsed and the resultingKeyObjecttype is compatible with thedsaEncodingoption in TypeScript's type definitions.♻️ Proposed fix to remove `@ts-ignore`
export class EcdsaSha256 implements SignatureAlgorithm { getSignature = createOptionalCallbackFunction( (signedInfo: crypto.BinaryLike, privateKey: crypto.KeyLike): string => { - // Maybe the fix for ts-ignore below? - // const parsedPrivateKey = crypto.createPrivateKey(privateKey); + const parsedPrivateKey = crypto.createPrivateKey(privateKey); const signer = crypto.createSign("SHA256"); signer.update(signedInfo); - // `@ts-ignore` - const res = signer.sign({ key: privateKey, dsaEncoding: 'ieee-p1363' }, "base64"); + const res = signer.sign({ key: parsedPrivateKey, dsaEncoding: 'ieee-p1363' }, "base64"); return res; }, );
159-187: Same@ts-ignoreissue applies toEcdsaSha512.Apply the same fix as recommended for
EcdsaSha256to maintain consistency and type safety.♻️ Proposed fix
export class EcdsaSha512 implements SignatureAlgorithm { getSignature = createOptionalCallbackFunction( (signedInfo: crypto.BinaryLike, privateKey: crypto.KeyLike): string => { - // Maybe the fix for ts-ignore below? - // const parsedPrivateKey = crypto.createPrivateKey(privateKey); + const parsedPrivateKey = crypto.createPrivateKey(privateKey); const signer = crypto.createSign("SHA512"); signer.update(signedInfo); - // `@ts-ignore` - const res = signer.sign({ key: privateKey, dsaEncoding: 'ieee-p1363' }, "base64"); + const res = signer.sign({ key: parsedPrivateKey, dsaEncoding: 'ieee-p1363' }, "base64"); return res; }, );example/result.xml (1)
1-1: Consider addingexample/result.xmlto.gitignore.This file is generated output from running the example script. Committing generated files can cause confusion if they become stale. Alternatively, if this serves as a reference/fixture, consider renaming it to indicate it's a sample output.
example/example.js (1)
10-11: Missing semicolons - inconsistent with file style.Lines 10-11 are missing semicolons while the rest of the file uses them consistently.
🔧 Proposed fix
- sig.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#" - sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256" + sig.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256";test/signature-unit-tests.spec.ts (1)
82-141: ECDSA test suite provides good coverage.The tests properly cover both signing/verification and manipulation detection for ECDSA algorithms. The structure mirrors the RSA tests, ensuring consistent behavior.
Consider extracting the common test logic into a shared helper to reduce duplication with the RSA test block (lines 22-80), but this is optional.
,
| const select = require("xpath").select | ||
| const dom = require("@xmldom/xmldom").DOMParser; | ||
| const SignedXml = require("../").SignedXml; | ||
| const fs = require("fs"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use package name import instead of relative path.
The import on line 6 uses a relative path (require("../")) rather than the package name. Example files should use require("xml-crypto") to demonstrate usage from an end-user's perspective, matching the pattern in example/example.js.
📦 Proposed fix
const select = require("xpath").select
const dom = require("@xmldom/xmldom").DOMParser;
-const SignedXml = require("../").SignedXml;
+const SignedXml = require("xml-crypto").SignedXml;
const fs = require("fs");Based on learnings: "Example files in the node-saml/xml-crypto repository should use require("xml-crypto") (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const select = require("xpath").select | |
| const dom = require("@xmldom/xmldom").DOMParser; | |
| const SignedXml = require("../").SignedXml; | |
| const fs = require("fs"); | |
| const select = require("xpath").select | |
| const dom = require("@xmldom/xmldom").DOMParser; | |
| const SignedXml = require("xml-crypto").SignedXml; | |
| const fs = require("fs"); |
🤖 Prompt for AI Agents
In `@example/local_example.js` around lines 4 - 7, Replace the relative
require("../") that imports SignedXml with the package name
require("xml-crypto") so the example demonstrates end-user usage; locate the
SignedXml import in local_example.js (currently assigned to the SignedXml
variable) and update that require to "xml-crypto" while leaving other requires
(select, DOMParser, fs) unchanged.
|
|
||
| verifySignature = createOptionalCallbackFunction( | ||
| (material: string, key: crypto.KeyLike, signatureValue: string): boolean => { | ||
| const publicKey = crypto.createPublicKey(key); |
There was a problem hiding this comment.
potential for key confusion attacks i.e. rsa public key being passed here
There was a problem hiding this comment.
I had looked into this, but the crypto functions seem to be validating this already. Do you mind demonstrating a key confusion attack?
There was a problem hiding this comment.
It's best practice to add checks (for this and the other signature-algorithms) that the verification key is the correct type (ec for this one). Currently there's no security vulnerability. If a rsa public key is passed in here, node:crypto will just verify with RSA (the key isn't converted to EC).
Moves basic ECDSA tests to a new, dedicated `ecdsa-signatures.spec.ts` file. The new suite adds comprehensive test cases for various scenarios, including key type mismatches, signature manipulation, external fixtures, and `KeyObject` verification, significantly improving coverage and robustness.
|
@madaster97 , I've reviewed this and created some tests that are currently failing. Please have another look at this code and see what you can do to bring it up to standard. Also, please run |
3 participants
Adds the feature I requested in #530 , support for the following algorithms (both signing and verifying):
I worked backwards from this example dotnet implementation (had a sample ecdsa-sha256 document). I added that sample document in a unit test as external confirmation that we verify these signatures correctly, and then made paired sign and verify - ecdsa tests that show that our signatures verify correctly.
Thanks to @uladkasach, for this stack overflow answer that showed how to call the crypto APIs correctly.
While I was here, I also touched up
example.js, which was missing some config requirements forcomputeSignature, like:sig.canonicalizationAlgorithmsig.signatureAlgorithmaddReference > digestAlgorithmaddReference > transformsSummary by CodeRabbit
New Features
Documentation
Tests